Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(swap): clean up Swap widget from isWrapOrUnwrap #2775

Merged
merged 18 commits into from
Jul 6, 2023
Merged

Conversation

shoom3301
Copy link
Collaborator

@shoom3301 shoom3301 commented Jul 3, 2023

Summary

Context: #2774

Removed wrap/unwrap checks from Swap page code, because wrap/unwrap flow is consolidated in TradeWidget.

To Test (Swap / Limit / Advanced)

  1. Wrap/Unwrap flow should work the same in all trade widgets
  2. ETH-flow (only for Swap)

@shoom3301 shoom3301 requested review from a team July 3, 2023 12:12
@vercel
Copy link

vercel bot commented Jul 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
swap-dev 🔄 Building (Inspect) Visit Preview

🌃 Cosmos ↗︎

@shoom3301 shoom3301 changed the base branch from develop to fix/2768 July 4, 2023 07:14
Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks and works good 👍

Only one behaviour change from prod which might be a problem, but not a big one.
It's no longer possible to insert the receive amount when wrapping/unwrapping.
Yes, the amounts are exactly the same, it has no actual difference from inputting it on sell or buy, but raising it just in case.

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see so many deletions of lines of code!

Soft approve, cause i haven't tested it. I would suggest @elena-zh tests this, but not the previous one. Because we might need to do quite some tests

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Swap page:
  • no waiting modal when run wrap/unwrap transaction
    waiting

like

  • not allowed to edit TO field's value
  • Custom recipient field should be hidden when wrap/unwrap
    image
  1. Limit orders:
  • error loading price for unwrap/wrap
    image
  • also impossible to edit To field's value
  • Custom recipient field should be hidden when wrap/unwrap
  1. Advanced: same comments as for Limit orders page (besides custom recipient field)

@shoom3301
Copy link
Collaborator Author

@elena-zh thanks! Fixed everything

@shoom3301
Copy link
Collaborator Author

not allowed to edit TO field's value

I did it intentionally to keep the code simple. I don't see any concrete reasons for this functionality

@elena-zh
Copy link

elena-zh commented Jul 5, 2023

Hey @shoom3301 , all is good, The last issue that I see that there is no balance check on Wrap/unwrap transaction. Could you please fix this as well?
no balance

Wrap/unwrap button should be disabled when 'could not load balances' and 'insufficient balance' errors are happening.

@shoom3301
Copy link
Collaborator Author

@elena-zh thanks! Fixed

@elena-zh
Copy link

elena-zh commented Jul 5, 2023

Hey @shoom3301 , the last nitpick: amount is reset to 0 when press on the change tokens arrow on the Swap page:
image

@shoom3301
Copy link
Collaborator Author

@elena-zh thanks, fixed!

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

… fix/2768-1

# Conflicts:
#	src/modules/tradeFormValidation/services/validateTradeForm.ts
#	src/pages/LimitOrders/index.tsx
@shoom3301 shoom3301 changed the base branch from fix/2768 to develop July 6, 2023 07:19
@shoom3301 shoom3301 merged commit a1d121e into develop Jul 6, 2023
5 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2023
Comment on lines -67 to +64
<TradeFormBlankButton onClick={context.wrapNativeFlow}>
<TradeFormBlankButton onClick={() => context.wrapNativeFlow()}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the wrapper fn?

@alfetopito alfetopito deleted the fix/2768-1 branch July 6, 2023 09:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants